EZP-29089: Usability bug when deleting containers #704

ViniTou · 2018-10-29T15:25:47Z

Question	Answer
Tickets	https://jira.ez.no/browse/EZP-29089
Bug fix?	no
New feature?	yes
BC breaks?	no
Tests pass?	yes
Doc needed?	no
License	GPL-2.0

Description

This checks if location is non-empty container and if it is displays additional info modal. If content have asset fieldtype additional modals are displayed after first one.

Checklist:

Coding standards ($ composer fix-cs)
Ready for Code Review

mikadamczyk · 2018-10-30T07:50:13Z

src/bundle/Resources/public/js/scripts/admin.trash.container.js

+    };
+
+    toggleForm.addEventListener('submit', openTrashImageAssetModal, false);
+


not needed empty line

mikadamczyk · 2018-10-30T08:03:13Z

src/lib/Specification/Location/HasChildren.php

+    {
+        $children = $this->locationService->loadLocationChildren($item);
+
+        return 0 !== $children->totalCount;


maybe 0 < $children->totalCount

sunpietro · 2018-10-30T08:31:33Z

src/bundle/Resources/public/js/scripts/admin.trash.container.js

@@ -0,0 +1,17 @@
+(function (global, doc) {
+    const toggleForm = doc.querySelector('form[name="location_trash_container"]');
+    const haveAsset = toggleForm.dataset.haveAsset;


sunpietro · 2018-10-30T08:31:49Z

src/bundle/Resources/public/js/scripts/admin.trash.container.js

+(function (global, doc) {
+    const toggleForm = doc.querySelector('form[name="location_trash_container"]');
+    const haveAsset = toggleForm.dataset.haveAsset;
+    const haveUniqueAsset = toggleForm.dataset.haveUniqueAsset;


hasUniqueAsset

should we not be consistent with how the corresponding class is named?

src/bundle/Resources/public/js/scripts/admin.trash.container.js

sunpietro · 2018-10-30T08:34:27Z

src/bundle/Resources/public/js/scripts/button.state.checkbox.toggle.js

@@ -0,0 +1,27 @@
+(function (global, doc) {
+    const toggleForms = [...doc.querySelectorAll('.ez-toggle-btn-state-checkbox')];


Due to latest changes in the browsers specification implementation, doc.querySelectorAll('.ez-toggle-btn-state-checkbox') is just enough. No need to convert it to an array to use with forEach only.

sunpietro · 2018-10-30T08:34:51Z

src/bundle/Resources/public/js/scripts/button.state.checkbox.toggle.js

+    const ANY_CHECKED = 'any-checked';
+
+    toggleForms.forEach((toggleForm) => {
+        const checkboxInputs = [...toggleForm.querySelectorAll('input[type="checkbox"]')];


Same as above.

sunpietro · 2018-10-30T08:38:20Z

src/bundle/Resources/public/js/scripts/button.state.checkbox.toggle.js

+
+    toggleForms.forEach((toggleForm) => {
+        const checkboxInputs = [...toggleForm.querySelectorAll('input[type="checkbox"]')];
+        const toggleButtonState = () => {


This function should be implemented outside of forEach loop. If you want to have an access to the checkboxInputs then a useful piece of information is that a forEach loop callback takes 3 params: item, index, an iterated array with all values. See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach

sunpietro · 2018-10-30T08:39:50Z

src/bundle/Resources/public/js/scripts/button.state.checkbox.toggle.js

+        const toggleButtonState = () => {
+            const button = doc.querySelector(toggleForm.dataset.toggleButtonId);
+
+            const anyIsChecked = checkboxInputs.some(el => el.checked);


CS: all variables should come one after another without additional empty lines between them.

tischsoic · 2018-10-30T12:16:42Z

src/bundle/Resources/views/content/modal_location_trash_container.html.twig

+                {{ form_widget(form.trashContainer) }}
+            </div>
+            <div class="modal-footer">
+                <button type="button" class="btn btn-secondary btn--no" data-dismiss="modal">


According to our style guidelines (https://doc.ezplatform.com/en/2.2/guidelines/components/buttons/) this button probably should have classes btn btn-dark.

sunpietro · 2018-10-30T13:42:23Z

src/bundle/Resources/public/js/scripts/button.state.checkbox.toggle.js

+
+    const toggleButtonState = (toggleForm, checkboxInputs) => {
+        const button = doc.querySelector(toggleForm.dataset.toggleButtonId);
+        const anyIsChecked = checkboxInputs.some(el => el.checked);


isAnyChecked

sunpietro · 2018-10-30T13:43:32Z

src/bundle/Resources/public/js/scripts/button.state.checkbox.toggle.js

+        const checkboxInputs = toggleForm.querySelectorAll('input[type="checkbox"]');
+
+        checkboxInputs.forEach((input, index, inputs) => {
+            input.addEventListener('change', toggleButtonState.bind(null, toggleForm, [...inputs]), false);


Do you need [...inputs]?

I can't use .some and .every on Node Collection, unless there is some other way to do this.

sunpietro · 2018-10-30T13:44:52Z

src/bundle/Resources/public/js/scripts/button.state.checkbox.toggle.js

+    const toggleButtonState = (toggleForm, checkboxInputs) => {
+        const button = doc.querySelector(toggleForm.dataset.toggleButtonId);
+        const anyIsChecked = checkboxInputs.some(el => el.checked);
+        const allChecked = checkboxInputs.every(el => el.checked);


I think this line and the line above can be moved into the line line no. 22 and 23, for better performance.

lserwatka · 2018-10-31T13:29:19Z

@mnocon Behat is failing here

lserwatka · 2018-10-31T13:30:07Z

Perhaps rebase is needed with master after your fix will be merged up.

lserwatka · 2018-10-31T14:26:14Z

@ViniTou could you rebase with master?

This comment has been minimized.

Sign in to view

ViniTou requested review from mikadamczyk, dew326, sunpietro, tischsoic and adamwojs and removed request for dew326 October 30, 2018 07:38

mikadamczyk approved these changes Oct 30, 2018

View reviewed changes

sunpietro suggested changes Oct 30, 2018

View reviewed changes

tischsoic reviewed Oct 30, 2018

View reviewed changes

tischsoic approved these changes Oct 30, 2018

View reviewed changes

sunpietro suggested changes Oct 30, 2018

View reviewed changes

sunpietro approved these changes Oct 30, 2018

View reviewed changes

ViniTou added the Ready for QA label Oct 30, 2018

katarzynazawada self-assigned this Oct 31, 2018

katarzynazawada approved these changes Oct 31, 2018

View reviewed changes

katarzynazawada added QA approved and removed Ready for QA labels Oct 31, 2018

EZP-29089: Usability bug when deleting containers

beecd27

lserwatka merged commit 6ea13bb into ezsystems:master Nov 2, 2018

ViniTou pushed a commit that referenced this pull request May 12, 2023

IBX-2778: Personalization - Wrong styles for Scenarios (#704)

16bcd73

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-29089: Usability bug when deleting containers #704

EZP-29089: Usability bug when deleting containers #704

ViniTou commented Oct 29, 2018 •

edited

Loading

This comment has been minimized.

mikadamczyk Oct 30, 2018

mikadamczyk Oct 30, 2018

sunpietro Oct 30, 2018

sunpietro Oct 30, 2018

ViniTou Oct 30, 2018

sunpietro Oct 30, 2018

sunpietro Oct 30, 2018

sunpietro Oct 30, 2018

sunpietro Oct 30, 2018

tischsoic Oct 30, 2018

sunpietro Oct 30, 2018

sunpietro Oct 30, 2018

ViniTou Oct 30, 2018

sunpietro Oct 30, 2018

lserwatka commented Oct 31, 2018

lserwatka commented Oct 31, 2018

lserwatka commented Oct 31, 2018

		};

		toggleForm.addEventListener('submit', openTrashImageAssetModal, false);

		@@ -0,0 +1,27 @@
		(function (global, doc) {
		const toggleForms = [...doc.querySelectorAll('.ez-toggle-btn-state-checkbox')];

EZP-29089: Usability bug when deleting containers #704

EZP-29089: Usability bug when deleting containers #704

Conversation

ViniTou commented Oct 29, 2018 • edited Loading

Description

Checklist:

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lserwatka commented Oct 31, 2018

lserwatka commented Oct 31, 2018

lserwatka commented Oct 31, 2018

ViniTou commented Oct 29, 2018 •

edited

Loading